Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cover all tests #1011

Merged
merged 10 commits into from
Jul 30, 2024
Merged

chore: cover all tests #1011

merged 10 commits into from
Jul 30, 2024

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Jun 19, 2024

Changes

This PR adds snapshot testing to ALL tests.

I also changed a bit the way we create snapshot testing, I created an utility called MakeSnapshot inside test_utils.go, which accepts certain options to generate the snapshots.

Before this PR, the snapshots were stored inside a __snapshots__ folder, but I preferred to configure that value, and pass a specific folder for each test suite we're testing. I found dividing snapshots by test case to be nice.

Testing

CI should pass

Docs

N/A

Copy link

changeset-bot bot commented Jun 19, 2024

⚠️ No Changeset found

Latest commit: 0ce3b62

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Base automatically changed from docs/better-testing to main June 20, 2024 07:41
@ematipico ematipico marked this pull request as ready for review June 20, 2024 07:50
@bluwy
Copy link
Member

bluwy commented Jun 24, 2024

Do we need snapshot testing for all tests? Seems like these are rather short and can be asserted entirely with a single string. The features are also well-defined and isolated (and unlikely to have internal implementation change).

I think it's fine to leave it as is, and perhaps leave snapshots for larger file outputs only that's hard to assert and susceptible to internal implementation change.

@ematipico
Copy link
Member Author

Do we need snapshot testing for all tests? Seems like these are rather short and can be asserted entirely with a single string. The features are also well-defined and isolated (and unlikely to have internal implementation change).

That's my question from me to you, as you have more knowledge than me, background-wise. I hope you can tell me which tests don't need a snapshot and why.

From what I can tell, ALL tests use a transformation step, which is the whole point of the snapshot.

@matthewp
Copy link
Contributor

matthewp commented Jul 1, 2024

The printer tests are the ones that should be a snapshot. But from this PR it doesn't look like the old manual tests are removed. So I don't think I am following what is going on.

@ematipico
Copy link
Member Author

The printer tests are the ones that should be a snapshot. But from this PR it doesn't look like the old manual tests are removed. So I don't think I am following what is going on.

  1. I'll reduce the amount of snapshots to the printing
  2. Regarding the old tests, I explained here why I found them useful: feat: snapshot testing (1/*) #995 (comment)

@matthewp
Copy link
Contributor

matthewp commented Jul 2, 2024

Regarding (2), I think you're saying that you find the print infrastructure useful for creating new tests. But once you've created them, there's not value in keeping them around forever, is there? If the existing printer want{} tests are duplicates of the snapshot tests then I don't see what value we are getting from the snapshots at all. I would be fine with keeping the print test infrastructure around as a helper during development, but don't see the benefit of checking in 2 copies of a testing.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check all the snapshots individually or anything, but this looks fine to me!

@ematipico ematipico merged commit 0ebfc96 into main Jul 30, 2024
5 checks passed
@ematipico ematipico deleted the feat/more-snapshot-tests branch July 30, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants